api: support channel archiving and channel list filters#291
Open
txchen wants to merge 1 commit intospacedriveapp:mainfrom
Open
api: support channel archiving and channel list filters#291txchen wants to merge 1 commit intospacedriveapp:mainfrom
txchen wants to merge 1 commit intospacedriveapp:mainfrom
Conversation
Contributor
WalkthroughAdds channel archiving and queryable channel listing. Introduces ListChannelsQuery and SetChannelArchiveRequest types, a PUT /channels/archive endpoint, updates list_channels to accept query params, and extends ChannelStore with list(is_active_filter) and set_active; includes unit tests for new behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/channels.rs (1)
69-111: Pushis_activefiltering into SQL to avoid over-fetching.On Line 82,
store.list(include_inactive)can fetch extra rows and then filter in memory on Line 85. For larger channel tables, this adds avoidable DB/network/memory overhead. Consider extendingChannelStorewith an optionalis_activepredicate so filtering happens at query time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/channels.rs` around lines 69 - 111, The current list_channels function calls ChannelStore::list(include_inactive) which fetches extra rows and then filters by query.is_active in-memory; change ChannelStore::list to accept an optional is_active filter (e.g., Option<bool> or a tri-state) so the database query can apply the active/inactive predicate. Update the call in list_channels to pass query.is_active (or convert the existing include_inactive logic into the new parameter) instead of include_inactive, remove the in-memory is_active filtering block, and ensure ChannelStore::list and its SQL/query builder use the new parameter to add a WHERE clause on is_active. Use the symbols ChannelStore::list, list_channels, include_inactive, and query.is_active to locate the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api/channels.rs`:
- Around line 69-111: The current list_channels function calls
ChannelStore::list(include_inactive) which fetches extra rows and then filters
by query.is_active in-memory; change ChannelStore::list to accept an optional
is_active filter (e.g., Option<bool> or a tri-state) so the database query can
apply the active/inactive predicate. Update the call in list_channels to pass
query.is_active (or convert the existing include_inactive logic into the new
parameter) instead of include_inactive, remove the in-memory is_active filtering
block, and ensure ChannelStore::list and its SQL/query builder use the new
parameter to add a WHERE clause on is_active. Use the symbols
ChannelStore::list, list_channels, include_inactive, and query.is_active to
locate the edits.
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/channels.rs (1)
76-113:⚠️ Potential issue | 🟡 MinorGlobal channel ordering is currently non-deterministic across agents.
Each agent’s SQL result is sorted, but the merged response order depends on map iteration order. If clients expect newest-first globally, this can produce unstable ordering.
💡 Proposed fix
pub(super) async fn list_channels( State(state): State<Arc<ApiState>>, Query(query): Query<ListChannelsQuery>, ) -> Json<ChannelsResponse> { @@ for (agent_id, pool) in pools.iter() { @@ } + + // Ensure deterministic newest-first ordering across all agents. + all_channels.sort_by(|left, right| right.last_activity_at.cmp(&left.last_activity_at)); Json(ChannelsResponse { channels: all_channels, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/channels.rs` around lines 76 - 113, The merged channel list in list_channels is non-deterministic because you append per-agent results in map iteration order; instead collect channels into a temporary vector of (agent_id, Channel) from each ChannelStore::list call, then sort that vector globally by channel.last_activity_at (descending, newest-first) with a secondary sort by channel.created_at (descending) to break ties, and only after sorting map items into ChannelResponse (formatting timestamps with to_rfc3339()). Update list_channels to build the temporary collection, perform the sort, then populate all_channels from the sorted collection so the response order is stable and newest-first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/api/channels.rs`:
- Around line 76-113: The merged channel list in list_channels is
non-deterministic because you append per-agent results in map iteration order;
instead collect channels into a temporary vector of (agent_id, Channel) from
each ChannelStore::list call, then sort that vector globally by
channel.last_activity_at (descending, newest-first) with a secondary sort by
channel.created_at (descending) to break ties, and only after sorting map items
into ChannelResponse (formatting timestamps with to_rfc3339()). Update
list_channels to build the temporary collection, perform the sort, then populate
all_channels from the sorted collection so the response order is stable and
newest-first.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/api/channels.rssrc/api/server.rssrc/conversation/channels.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/server.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/conversation/channels.rs (1)
228-228: Use a non-abbreviated error variable name.At Line 228, prefer
|error|over|e|for readability and guideline consistency.As per coding guidelines "Do not abbreviate variable names; use
queuenotq,messagenotmsg,channelnotch; common abbreviations likeconfigare acceptable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/channels.rs` at line 228, Replace the single-letter closure variable in the map_err call with a descriptive name: change the closure pattern in the expression that currently uses |e| (the map_err closure invoking anyhow::anyhow!(e)) to use |error| and pass that into anyhow::anyhow!(error) so the map_err invocation becomes more readable; locate the map_err on the type-conversion/propagation chain in channels.rs (the expression ending with .map_err(|e| anyhow::anyhow!(e))?) and update the closure variable and its usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/conversation/channels.rs`:
- Line 228: Replace the single-letter closure variable in the map_err call with
a descriptive name: change the closure pattern in the expression that currently
uses |e| (the map_err closure invoking anyhow::anyhow!(e)) to use |error| and
pass that into anyhow::anyhow!(error) so the map_err invocation becomes more
readable; locate the map_err on the type-conversion/propagation chain in
channels.rs (the expression ending with .map_err(|e| anyhow::anyhow!(e))?) and
update the closure variable and its usage accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/api/channels.rssrc/api/server.rssrc/conversation/channels.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/server.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds non-destructive channel management for large deployments with many threads/channels.
What’s added
GET /api/channelsnow supports optional query filters:include_inactive(bool): include archived/inactive channelsagent_id(string): restrict results to a single agentis_active(bool): return only active or only inactive channelsPUT /api/channels/archiveagent_id(string)channel_id(string)archived(bool)ChannelStoreenhancements:list(include_inactive: bool)set_active(channel_id, active)list_active()remains and now delegates tolist(false)for backward compatibility.Why
Deleting channels currently removes message history, which is too destructive for UI/thread-management use cases.
This PR enables hiding old channels from normal listings while retaining history.
Behavior notes
channels.is_active = 0; unarchiving sets it back to1.DELETE /api/channelsstill deletes channel + history).Tests
Added unit tests in
src/conversation/channels.rs:list_include_inactive_controls_visibilityset_active_toggles_channel_state_without_deletingAlso verified:
cargo check --libcargo fmt --all --checkcargo test conversation::channels::tests -- --nocapture